[Browser MFA] Add protobuf and config#63831
Conversation
8e9cfa5 to
f5df2e4
Compare
| message ValidateBrowserMFAChallengeResponse { | ||
| // tsh_redirect_url is the callback URL to tsh's local HTTP server with the encrypted WebAuthn response. | ||
| // Format: http://127.0.0.1:[random_port]/callback?response={encrypted_webauthn_response} | ||
| string tsh_redirect_url = 1; |
There was a problem hiding this comment.
I don't think it was mentioned in the RFD, but will there be any protection against creating an open-redirect logic on the web frontend side?
There was a problem hiding this comment.
I wasn't planning on adding redirect validation on the web frontend because the redirect url is accepted once from the user at the start of the ceremony and validated by ValidateClientRedirect. The URL is then stored on the backend waiting for the user to MFA in the browser. After doing so, the auth server encrypts the WebAuthn response and adds it to the stored and validated redirect URL for returning to the browser.
| // AllowBrowserAuthentication enables/disables browser-based authentication. | ||
| // When set to false, authentication flows that require a browser will be disabled. | ||
| // Defaults to true. |
There was a problem hiding this comment.
The way it's phrased suggests that any kind of local login (including regular sign-ins to the web app) would be gated by this option. Is that the case? If not, the documentation should be a bit more clear that we're only talking about the browser MFA flow for tsh.
There was a problem hiding this comment.
That is not the case, this is only available to CLI tools. I'll make sure this is clearer in this comment and in documentation.
| string proxy_address_for_sso = 4; | ||
| // Used to construct the redirect URL for browser-based MFA flows. If the client supports browser MFA, this field | ||
| // should be set to the URL where the browser should redirect to tsh after completing the MFA challenge. | ||
| string browser_mfa_tsh_redirect_url = 5; |
There was a problem hiding this comment.
Will this URL be validated by the auth server?
There was a problem hiding this comment.
Yes, this will also be validated by ValidateClientRedirect on the server side
| // per-session MFA but we may still need to know that the user has TOTP | ||
| // configured as an option. | ||
| bool per_session_mfa = 7; | ||
| BrowserMFAChallenge browser = 8; |
There was a problem hiding this comment.
What's the role of this field? I also see it mentioned in the RFD, but I don't understand how this all relates to teleterm.
There was a problem hiding this comment.
When a user initiates a request in teleterm that requires MFA, a request is sent by tsh daemon to Teleport and Teleport replies with the available challenges. The daemon will then create this PromptMFARequest message and forward it to teleterm, which will then show a modal for the user to choose their method of MFA. So this field just lets teleterm know that Browser MFA is an MFA option
|
Amplify deployment status
|
| case c.Spec.AllowBrowserAuthentication == nil: | ||
| // Materialize AllowBrowserAuthentication to true if WebAuthn is enabled, | ||
| // otherwise leave it nil. | ||
| if hasWebauthn { | ||
| c.Spec.AllowBrowserAuthentication = NewBoolOption(true) | ||
| } |
There was a problem hiding this comment.
| case c.Spec.AllowBrowserAuthentication == nil: | |
| // Materialize AllowBrowserAuthentication to true if WebAuthn is enabled, | |
| // otherwise leave it nil. | |
| if hasWebauthn { | |
| c.Spec.AllowBrowserAuthentication = NewBoolOption(true) | |
| } | |
| case hasWebauthn && c.Spec.AllowBrowserAuthentication == nil: | |
| // Materialize AllowBrowserAuthentication to true if WebAuthn is enabled, | |
| // otherwise leave it nil. | |
| c.Spec.AllowBrowserAuthentication = NewBoolOption(true) |
It also occurs to me that we could never materialize and always calculate on the fly. Maybe add a variant of GetAllowBrowserAuthentication() that does that, so the logic is all captured here.
There was a problem hiding this comment.
Sounds good to me, left a validation check in CheckAndSetDefaults and calculate the rest on the fly in the getter
|
Looks good, a few minor comments only. |
e08f5d2 to
55869d8
Compare
2c63567 to
8ea0de0
Compare
8ea0de0 to
0cb377a
Compare
Improve comments grpc, lint, test fix Update tf docs Update tf integration Fix login test Address comments Fix test Move ValidateBrowserMFAChallenge rpc to MFA service Fix service test Address comments Don't materialise browser authentication Remove browser auth materialisation in test Convert validate browser mfa to complete browser mfa
0cb377a to
8a5561b
Compare
|
@danielashare See the table below for backport results.
|
Improve comments grpc, lint, test fix Update tf docs Update tf integration Fix login test Address comments Fix test Move ValidateBrowserMFAChallenge rpc to MFA service Fix service test Address comments Don't materialise browser authentication Remove browser auth materialisation in test Convert validate browser mfa to complete browser mfa
[Browser MFA] Add protobuf and config (#63831) [Browser MFA] Add proto for Browser MFA feature (#64048) [Browser MFA] Add CompleteBrowserMFAChallenge gRPC (#63873) [Browser MFA] Rename browser mfa config name (#64980) [Browser MFA] Add BrowserMFARequestID to CreateAuthenticateChallenge (#63945) [Browser MFA] Add Browser MFA to challenge request flow (#63936) [Browser MFA] Add initial requests for browser MFA process to client tools (#64301) [Browser MFA] Add tsh callback handling for webauthn response (#64461) [Browser MFA] Add Browser MFA to presence checks (#65052) [Browser MFA] Add browser MFA path to MFA finish flow (#64523) [Browser MFA] Add Browser MFA to Connect (#64887) [Browser MFA] Add Browser MFA UI (#64692) [Browser MFA] Fix formatting in moderated sessions (#65236) [Browser MFA] Add Browser MFA ceremony tests
[Browser MFA] Add protobuf and config (gravitational#63831) [Browser MFA] Add proto for Browser MFA feature (gravitational#64048) [Browser MFA] Add CompleteBrowserMFAChallenge gRPC (gravitational#63873) [Browser MFA] Rename browser mfa config name (gravitational#64980) [Browser MFA] Add BrowserMFARequestID to CreateAuthenticateChallenge (gravitational#63945) [Browser MFA] Add Browser MFA to challenge request flow (gravitational#63936) [Browser MFA] Add initial requests for browser MFA process to client tools (gravitational#64301) [Browser MFA] Add tsh callback handling for webauthn response (gravitational#64461) [Browser MFA] Add Browser MFA to presence checks (gravitational#65052) [Browser MFA] Add browser MFA path to MFA finish flow (gravitational#64523) [Browser MFA] Add Browser MFA to Connect (gravitational#64887) [Browser MFA] Add Browser MFA UI (gravitational#64692) [Browser MFA] Fix formatting in moderated sessions (gravitational#65236) [Browser MFA] Add Browser MFA ceremony tests
This PR adds protobuf and the configuration for enabling Browser MFA. The RFD for these additions can be found here.
Manual tests: